Refactor the constraint set to avoid copying for concentration by boulderdaze · Pull Request #949 · NCAR/micm

boulderdaze · 2026-03-16T18:19:02Z

This PR:

Addresses the review comments in DAE constraint restructure to full mass-matrix row replacement #931.
Refactors ConstraintSet into a templated class parameterized as ConstraintSet<DenseMatrixPolicy, SparseMatrixPolicy>
Precompiles residual/Jacobian functions via SetConstraintFunctions

Addressing the following comments:

Avoid copying of concentrations
Move the indexing logic into Residual function
Specialize functions for the various matrix types (as done in ProcessSet), and use the new DenseMatrixPolicy::Function( and SparseMatrixPolicy::Function().
Move the specific constraints (linear, equilibrium) into a sub-folder, similar to how rate constants are in a sub-folder of process/.

include/micm/constraint/types/
├── equilibrium_constraint.hpp
└── linear_constraint.hpp

I will add Matt as a reviewer after the team completes their review, and after I address any comments.

Copilot

Pull request overview

This PR refactors MICM’s algebraic constraint implementation to better integrate with the solver/build pipeline: constraints are moved into constraint/types, ConstraintSet is templated on dense/sparse matrix policies, and constraint evaluation is switched to precompiled function objects (similar to other MICM “Function(...)” patterns). Solver templates/builders and tests are updated accordingly.

Changes:

Introduces micm::ConstraintInfo, templated ConstraintSet<DenseMatrixPolicy, SparseMatrixPolicy>, and precompiled residual/Jacobian functions via SetConstraintFunctions.
Moves LinearConstraint / EquilibriumConstraint headers into include/micm/constraint/types/* and updates includes/usages across solvers/tests.
Updates Rosenbrock/BackwardEuler solver template signatures (and CPU/CUDA wrappers) to carry a ConstraintSetPolicy.

Reviewed changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
test/unit/solver/test_rosenbrock.cpp	Updates constraint include path to new `constraint/types` location.
test/unit/solver/test_backward_euler.cpp	Updates solver policy instantiation to include `ConstraintSetPolicy`.
test/unit/constraint/type/test_linear.cpp	Adds new unit/integration-style tests for `LinearConstraint` via `ConstraintSet`.
test/unit/constraint/type/test_equilibrium.cpp	Adds new unit/integration-style tests for `EquilibriumConstraint` via `ConstraintSet`.
test/unit/constraint/type/CMakeLists.txt	Registers new constraint “type” unit tests.
test/unit/constraint/test_linear.cpp	Removes old linear constraint unit test file (replaced by type tests).
test/unit/constraint/test_equilibrium_constraint.cpp	Removes old equilibrium constraint unit test file (replaced by type tests).
test/unit/constraint/test_constraint_set.cpp	Refactors constraint set tests to use a shared policy header and cover multiple matrix policies.
test/unit/constraint/test_constraint_set_policy.hpp	Adds templated/shared test helpers for `ConstraintSet` across matrix policy variants.
test/unit/constraint/CMakeLists.txt	Updates unit-test wiring; adds `type/` subdirectory.
test/integration/test_linear_constraint.cpp	Updates includes to new `constraint/types` headers.
test/integration/test_equilibrium_constraint.cpp	Updates include path; removes a direct `ConstraintSet` API integration test; updates one instantiation to explicit template args.
test/integration/CMakeLists.txt	Renames/adjusts integration test target naming for equilibrium constraint test.
include/micm/solver/solver_builder.inl	Updates builder to use `SpeciesDependencies()` / `AlgebraicSpecies()` and to instantiate a templated `ConstraintSetPolicy`; adds `SetConstraintFunctions`.
include/micm/solver/solver_builder.hpp	Updates equilibrium constraint include to new `constraint/types` path.
include/micm/solver/rosenbrock.inl	Extends solver templates to include `ConstraintSetPolicy` and wires constraints with updated member type.
include/micm/solver/rosenbrock.hpp	Adds `ConstraintSetPolicy` template parameter; stores constraints as that policy type.
include/micm/solver/rosenbrock_solver_parameters.hpp	Updates solver type alias to include `ConstraintSetPolicy`.
include/micm/solver/backward_euler.inl	Extends backward-euler templates to include `ConstraintSetPolicy`.
include/micm/solver/backward_euler.hpp	Stores constraints as `ConstraintSetPolicy` and updates constructor signature.
include/micm/solver/backward_euler_solver_parameters.hpp	Updates solver type alias to include `ConstraintSetPolicy`.
include/micm/cuda/solver/cuda_rosenbrock.hpp	Extends CUDA Rosenbrock solver template parameters to include `ConstraintSetPolicy`.
include/micm/CPU.hpp	Updates CPU solver type aliases to pass `ConstraintSet<...>` as the new third template argument.
include/micm/constraint/types/linear_constraint.hpp	Adds new function-based `LinearConstraint` implementation for `ConstraintSet` integration.
include/micm/constraint/types/equilibrium_constraint.hpp	Adds new function-based `EquilibriumConstraint` implementation for `ConstraintSet` integration.
include/micm/constraint/linear_constraint.hpp	Removes legacy “pointer-based” linear constraint API.
include/micm/constraint/equilibrium_constraint.hpp	Removes legacy “pointer-based” equilibrium constraint API.
include/micm/constraint/constraint.hpp	Updates `Constraint` facade to new `SpeciesDependencies()` / `AlgebraicSpecies()` and function-object APIs.
include/micm/constraint/constraint_set.hpp	Reworks `ConstraintSet` into a dense/sparse policy template and adds precompiled forcing/Jacobian function storage.
include/micm/constraint/constraint_info.hpp	Introduces `ConstraintInfo` shared metadata for precompiled constraint functions.
include/micm/Constraint.hpp	Updates umbrella header to include new constraint type headers and updated constraint facade.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

include/micm/constraint/types/linear_constraint.hpp

include/micm/constraint/types/equilibrium_constraint.hpp

include/micm/cuda/solver/cuda_rosenbrock.hpp

include/micm/constraint/constraint_set.hpp

test/unit/constraint/test_constraint_set_policy.hpp

codecov-commenter · 2026-03-18T00:35:58Z

Codecov Report

❌ Patch coverage is 95.17241% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.87%. Comparing base (032b30e) to head (ce4a4ac).

Files with missing lines	Patch %	Lines
...e/micm/constraint/types/equilibrium_constraint.hpp	94.11%	9 Missing ⚠️
include/micm/constraint/constraint_set.hpp	93.22%	4 Missing ⚠️
...nclude/micm/constraint/types/linear_constraint.hpp	98.07%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #949      +/-   ##
==========================================
- Coverage   95.94%   95.87%   -0.08%     
==========================================
  Files          65       65              
  Lines        3870     3973     +103     
==========================================
+ Hits         3713     3809      +96     
- Misses        157      164       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

K20shores

Would you please rebase onto main or cherry-pick this branch so that we don't have all of the old commits?

K20shores

This does indeed remove the copies, to my understanding. Well done!

mattldawson

This looks great! Really nice work!

I also don't see any copies/allocations in the solver functions, and the usage of the matrix functions looks perfect!

I also really like the implementation of the ConstraintSet. It seems very similar to the structure of the external model API for processes. Maybe with some minor changes we would be able to use this for external constraints as well? The only thing I see right now that might prevent that is the use of a variant of constraint types that exist in MICM. But, that seems like it might be solvable.

One thing I missed in the original PR is that there is just a constant value for the equilibrium constant in the equilibrium constraint. Unfortunately, the equilibrium "constant" (like so many "constants" in chemistry) is not actually constant, it typically changes with temperature. I wouldn't try to address that in this PR, but I would suggest creating a follow-up PR. Allowing for temperature-dependent equilibrium constants will probably involve adopting more of the structure used for processes in the external model API where custom parameters can be created for constraints, their values can be updated when conditions change, and the forcing/jacobian (or equivalent constraint functions) get passed state variables AND custom parameters, instead of just state variables. I think this should be fairly straightforward, but would probably be a large enough change to warrant a separate PR.

Super nice implementation though!

boulderdaze · 2026-03-20T21:20:41Z

This looks great! Really nice work!

I also don't see any copies/allocations in the solver functions, and the usage of the matrix functions looks perfect!

I also really like the implementation of the ConstraintSet. It seems very similar to the structure of the external model API for processes. Maybe with some minor changes we would be able to use this for external constraints as well? The only thing I see right now that might prevent that is the use of a variant of constraint types that exist in MICM. But, that seems like it might be solvable.

One thing I missed in the original PR is that there is just a constant value for the equilibrium constant in the equilibrium constraint. Unfortunately, the equilibrium "constant" (like so many "constants" in chemistry) is not actually constant, it typically changes with temperature. I wouldn't try to address that in this PR, but I would suggest creating a follow-up PR. Allowing for temperature-dependent equilibrium constants will probably involve adopting more of the structure used for processes in the external model API where custom parameters can be created for constraints, their values can be updated when conditions change, and the forcing/jacobian (or equivalent constraint functions) get passed state variables AND custom parameters, instead of just state variables. I think this should be fairly straightforward, but would probably be a large enough change to warrant a separate PR.

Super nice implementation though!

Thank you for the guidance on this task (applying the matrix functions that you've recently introduced) and your suggestions! I created two PRs to follow up on your review comments:

boulderdaze self-assigned this Mar 16, 2026

boulderdaze requested a review from Copilot March 18, 2026 00:28

Copilot started reviewing on behalf of boulderdaze March 18, 2026 00:28 View session

Copilot AI reviewed Mar 18, 2026

View reviewed changes

boulderdaze marked this pull request as ready for review March 18, 2026 01:10

boulderdaze requested review from K20shores and dwfncar March 18, 2026 01:43

K20shores requested changes Mar 18, 2026

View reviewed changes

contains all the changes

5936037

boulderdaze force-pushed the rework-constraint-set branch from 64409a1 to 5936037 Compare March 18, 2026 16:25

boulderdaze requested a review from K20shores March 18, 2026 17:21

add gtest namespace

0e2d118

K20shores approved these changes Mar 18, 2026

View reviewed changes

merge main

ce4a4ac

boulderdaze requested a review from mattldawson March 19, 2026 01:07

mattldawson approved these changes Mar 20, 2026

View reviewed changes

boulderdaze merged commit 686000d into main Mar 20, 2026
26 checks passed

boulderdaze deleted the rework-constraint-set branch March 20, 2026 21:28

boulderdaze mentioned this pull request Mar 23, 2026

Store Jacobian buffer as ConstraintSet member or a solver temporary variables to avoid reallocating on every solver call #946

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the constraint set to avoid copying for concentration#949

Refactor the constraint set to avoid copying for concentration#949
boulderdaze merged 3 commits intomainfrom
rework-constraint-set

boulderdaze commented Mar 16, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Mar 18, 2026 •

edited

Loading

Uh oh!

K20shores left a comment

Uh oh!

K20shores left a comment

Uh oh!

mattldawson left a comment

Uh oh!

boulderdaze commented Mar 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

boulderdaze commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

K20shores left a comment

Choose a reason for hiding this comment

Uh oh!

K20shores left a comment

Choose a reason for hiding this comment

Uh oh!

mattldawson left a comment

Choose a reason for hiding this comment

Uh oh!

boulderdaze commented Mar 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

boulderdaze commented Mar 16, 2026 •

edited

Loading

codecov-commenter commented Mar 18, 2026 •

edited

Loading